-
Notifications
You must be signed in to change notification settings - Fork 167
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Added support for loading files as parameter values, including interpolation #185
Conversation
… for b64 and UserData-friendly Fn::Join blocks
All - I don't know why the tests above are failing. They are seemingly unrelated to my changes, and caused by util.pt still importing a boto2 module? |
Hey @ttarhan - this looks cool, thanks! A couple things:
Thanks for this - I'll dig a bit more into the code soon, but after a quick once over it looks fine to me. |
Thanks for the quick review, @phobologic. I'll work on a test and the doc update, and update the PR soon. |
hey @ttarhan - when you have a chance, can you merge master into your PR here? It should fix the testing issue you're hitting. |
Good morning, @phobologic -- that seems to have done the trick |
Awesome - thanks to @acmcelwee for the quick fix. We'll be working on one that lets us ditch boto2 more fully again in the future :) |
Hey @ttarhan - just wanted to ping you and remind you that this PR only needs the documentation & tests. I'm actually almost done fixing the boto3/boto2 issue in #192, which will also involve adding some more libraries for testing, in case those will help you here too. I'm planning to add: http://pythonhosted.org/testfixtures/ - In my use case I'm planning to use the LogCapture feature. I should have that PR finished up today. |
@phobologic - yeah, as soon as I get a free moment; should be this week. |
Awesome, thanks :) |
Oh, one other thing in regards to #188 - can you go ahead and bump the |
@phobologic - I added some tests. Please see if these are to your liking; Python isn't my everyday specialty, so I apologize for any issues there. |
|
||
|
||
class TestFileTranslator(unittest.TestCase): | ||
def setUp(self): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you pass on this, you can go ahead and just remove it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ha, oops. I thought I'd use it, so I put it there, and never removed it when it went unused.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've done that plenty of times myself :)
Looks good! If you could go ahead and remove the |
Ping @ttarhan - it looks like there are some conflicts now (though they're probably minor). Can you refresh from master, and add to the docs/remove the unused setUp and I'll go ahead and merge this? Hoping to get it into the next release. |
Hey @ttarhan, I hate to pull the rug from under this PR, but we've deprecated translators in favor of a more powerful concept called "Lookups". You can see how we converted the For some background on the change you can read through here: #194. The conversion should mostly just entail moving: Let me know if you have any questions! |
Ok. I'll do this tomorrow. |
@phobologic and @mhahn: I've got this basically ready, but issues #214 and #213 are preventing me from finishing this change (from translators to lookups). I'm not sure how to address them, especially #214. Let me know where we go from here. |
@phobologic and @mhahn - I've updated my branch with my progress, but this isn't ready to merge due to #214. Also, can you confirm my change in stacker/util.py looks right -- I'm shocked nobody else ran into an issue with that import. |
ah @ttarhan good catch with the import. when i moved those functions over to utils i didn't change the import path and i haven't used this anywhere i use that function yet. |
We should be. I'll test and report back shortly. |
You know, one more thought: we probably want to enhance the interpolation inside the parameterized codec here to support all variables. It currently assumes that any {{Variable}} is a CFN Param (and that was a fair assumption for the most part, until the new variable functionality). Now, we probably need to support both types of interpolation. But perhaps merge this PR and I'll do that work in a follow-up? |
|
||
|
||
class TestFileTranslator(unittest.TestCase): | ||
def setUp(self): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can remove this, since you pass.
Other than the setUp method, this looks great. I agree it'd probably be worth it to combine this with the new variable functionality as well, but that can come after we release .8 |
Ok, removed the setUp method |
Thanks, and thanks for sticking with this as we move towards Variables/Lookups. |
Adds a translator to take a filename and return the file contents, optionally encoding or interpolating the input
Fields should use the following format:
For example:
Supported codecs:
- plain
- base64 - encode the plain text file at the given path with base64 prior to returning it
- parameterized - the same as plain, but additionally supports referencing template parameters to create userdata that's supplemented with information from the template, as is commonly needed in EC2 UserData.
For example, given a template parameter of BucketName, the file could contain the following text:
and then you could use something like this in the YAML config file:
resulting in the UserData parameter being defined as:
When using parameterized-b64 for UserData, you should use a local_parameter defined as such:
and then assign UserData in a LaunchConfiguration or Instance to self.local_parameters["UserData"]